Skip to content

drivers: video: ov7670: update init regs #94354

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mjs513
Copy link

@mjs513 mjs513 commented Aug 11, 2025

This is in place of PR #94183 which:

While testing the OV7670 on the Arduino Giga using the ArduinoCore zephyr IDE I found that if:

{OV7670_COM10, 0x03}, /* COM10 */]

is not updated from

{OV7670_COM10, 0x20}, /* COM10 */

camera will start but no images will be shown.

I found this when looking at drivers: video: ov7670: Implement missing video API functions, controls and update init regs. it seems that only part of the PR was incorporated for the register changes requested?

Nice and clean - I hate git!

@avolmat-st
Copy link

Thanks @mjs513, I commented in a related PR which is tackling the same issue. Cf #92645

@mjs513
Copy link
Author

mjs513 commented Aug 11, 2025

@avolmat-st

Just had something strange occur. When I last test the 7670 was with arduinocore 3,1 with 4.2-rc. And it worked with just the change mention. Testing now with 3.2 and 4.2 (zephyr) I had to also change reset and power pins config, i.e.,

#if DT_ANY_INST_HAS_PROP_STATUS_OKAY(pwdn_gpios)
	/* Power up camera module */
	if (config->pwdn.port != NULL) {
		if (!gpio_is_ready_dt(&config->pwdn)) {
			return -ENODEV;
		}
		ret = gpio_pin_configure_dt(&config->pwdn, GPIO_OUTPUT_ACTIVE);
		if (ret < 0) {
			LOG_ERR("Could not clear power down pin: %d", ret);
			return ret;
		}
	}
	gpio_pin_set_dt(&config->reset, 1);
#endif
#if DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios)
	/* Reset camera module */
	if (config->reset.port != NULL) {
		if (!gpio_is_ready_dt(&config->reset)) {
			return -ENODEV;
		}
		ret = gpio_pin_configure_dt(&config->reset, GPIO_OUTPUT);
		if (ret < 0) {
			LOG_ERR("Could not set reset pin: %d", ret);
			return ret;
		}
		/* Reset is active low, has 1ms settling time*/
		gpio_pin_set_dt(&config->reset, 0);
		k_msleep(1);
		gpio_pin_set_dt(&config->reset, 1);
		k_msleep(1);
		gpio_pin_set_dt(&config->reset, 0);
		k_msleep(1);
	}
#endif

have you run into any issues like this - not sure want to include in this pr or make another. Any guidance would be helpful

Found this issue when looking at how to combine the 7675 driver wthi the ov7670 driver.

Thanks
Mike

@avolmat-st
Copy link

Hi @mjs513

not sure to fully see the difference you are highlighting, but I think I understand that you had to change the pwdn part from INACTIVE to ACTIVE and in the reset part you added a reset set to 0 at the end. Is that so ?
(for next time, could you share a diff rather than just your code since it makes it hard to understand the differences)

If that is so, this seems to be a difference in polarity. Basically in your case, the powerdown bit has to be set to high at the end and the reset line has to be set to 0 at the end. Is that correct ?

This code depends on the setting done within the device-tree for this module. Taking the dvp_20pin_ov7670 as an example and the file dvp_20pin_ov7670.overlay, the important configuration for this part is the two lines of gpio:

                reset-gpios = <&dvp_20pin_connector DVP_20PIN_PEN GPIO_ACTIVE_HIGH>;
                pwdn-gpios  = <&dvp_20pin_connector DVP_20PIN_PDN GPIO_ACTIVE_HIGH>;

This indicates that the reset and pwdn gpio are both active high. This means that
ret = gpio_pin_configure_dt(&config->pwdn, GPIO_OUTPUT_ACTIVE); will the the pwdn pin to logic 1 while it would have set it to 0 if was set as GPIO_ACTIVE_LOW in the device-tree.

Since you are introducing a new sensor (and possibily a new shield ?) then you should be able to configure that directly via the device-tree gpio configuration.

@mjs513
Copy link
Author

mjs513 commented Aug 12, 2025

@avolmat-st

My apologies for not showing the diff:


			return -ENODEV;
		}
		ret = gpio_pin_configure_dt(&config->pwdn, GPIO_OUTPUT_INACTIVE);
		if (ret < 0) {
		ret = gpio_pin_configure_dt(&config->pwdn, GPIO_OUTPUT_ACTIVE);
    		if (ret < 0) {
			LOG_ERR("Could not clear power down pin: %d", ret);
			return ret;
		}
	}
  //gpio_pin_set_dt(&config->pwdn, 1);
#endif
#if DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios)
	/* Reset camera module */

@@ -520,7 +521,7 @@ static int ov7670_init(const struct device *dev)
		if (!gpio_is_ready_dt(&config->reset)) {
			return -ENODEV;
		}
		ret = gpio_pin_configure_dt(&config->reset, GPIO_OUTPUT);
		ret = gpio_pin_configure_dt(&config->reset, GPIO_OUTPUT_ACTIVE);
		if (ret < 0) {
			LOG_ERR("Could not set reset pin: %d", ret);
			return ret;

@@ -530,6 +531,8 @@ static int ov7670_init(const struct device *dev)
		k_msleep(1);
		gpio_pin_set_dt(&config->reset, 1);
		k_msleep(1);
		gpio_pin_set_dt(&config->reset, 0);
		k_msleep(1);
	}
#endif

and in the overlay I have

		reset-gpios = <&gpiod 4 GPIO_ACTIVE_HIGH>;
		pwdn-gpios = <&gpioa 1 GPIO_ACTIVE_HIGH>;

Right now I am working on the combined ov7670 and ov7675. In the process of rewriting the OV7670 piece to use the video_write_cci etc commands in video_common. It will be cleaner and consistent. Not sure how to do the diff on that one :)

@avolmat-st
Copy link

@avolmat-st

My apologies for not showing the diff:


			return -ENODEV;
		}
		ret = gpio_pin_configure_dt(&config->pwdn, GPIO_OUTPUT_INACTIVE);
		if (ret < 0) {
		ret = gpio_pin_configure_dt(&config->pwdn, GPIO_OUTPUT_ACTIVE);
    		if (ret < 0) {
			LOG_ERR("Could not clear power down pin: %d", ret);
			return ret;
		}
	}
  //gpio_pin_set_dt(&config->pwdn, 1);
#endif
#if DT_ANY_INST_HAS_PROP_STATUS_OKAY(reset_gpios)
	/* Reset camera module */

@@ -520,7 +521,7 @@ static int ov7670_init(const struct device *dev)
		if (!gpio_is_ready_dt(&config->reset)) {
			return -ENODEV;
		}
		ret = gpio_pin_configure_dt(&config->reset, GPIO_OUTPUT);
		ret = gpio_pin_configure_dt(&config->reset, GPIO_OUTPUT_ACTIVE);
		if (ret < 0) {
			LOG_ERR("Could not set reset pin: %d", ret);
			return ret;

@@ -530,6 +531,8 @@ static int ov7670_init(const struct device *dev)
		k_msleep(1);
		gpio_pin_set_dt(&config->reset, 1);
		k_msleep(1);
		gpio_pin_set_dt(&config->reset, 0);
		k_msleep(1);
	}
#endif

and in the overlay I have

		reset-gpios = <&gpiod 4 GPIO_ACTIVE_HIGH>;
		pwdn-gpios = <&gpioa 1 GPIO_ACTIVE_HIGH>;

Right now I am working on the combined ov7670 and ov7675. In the process of rewriting the OV7670 piece to use the video_write_cci etc commands in video_common. It will be cleaner and consistent. Not sure how to do the diff on that one :)

Thanks for sharing. However I cannot see the usual - / + lines in your diff. Maybe just sharing the git diff output should be ok.
Anyway this really sounds like a polarity difference to me.

@mjs513
Copy link
Author

mjs513 commented Aug 12, 2025

Sorry didnt notive was doing it from Github desktop. Yep - going to test without the shield oh the gig and h7 after more editing. PS heres a image just for completeness
image

Copy link

@JarmouniA JarmouniA marked this pull request as draft August 12, 2025 20:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Video Video subsystem size: XS A PR changing only a single line of code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants